Skip to content

Add unit testing#18

Merged
Akash-Maurya-0899 merged 4 commits into
gwnrtools:masterfrom
prayush:add_unit_testing
Jun 12, 2026
Merged

Add unit testing#18
Akash-Maurya-0899 merged 4 commits into
gwnrtools:masterfrom
prayush:add_unit_testing

Conversation

@prayush

@prayush prayush commented May 8, 2026

Copy link
Copy Markdown
Contributor

This PR adds substantial new unit test coverage for waveform generation, waveform utilities, blending helpers, and legacy fitting functions.

Generator and mode-generation tests

New mocked tests were added for inspiral and merger-ringdown mode generation using patched lalsimulation and delegated generators. These tests verify:

  • correct handling of requested (l,m) modes,
  • inclusion/exclusion of conjugate modes,
  • correct return types (pycbc.TimeSeries vs NumPy arrays),
  • proper distance conversion before C-layer calls,
  • avoidance of duplicate mode generation calls,
  • correct eccentricity index selection logic,
  • correct error propagation for missing approximants,
  • filtering of requested modes from linked-list structures,
  • correct routing between lalsimulation and pyseobnr backends,
  • default handling of f_ref and coa_phase.

Deterministic numerical tests

Additional pure-Python tests (without mocks) were added for utils.py, blend.py, and legacy.py.

These tests verify:

  • consistency and scaling relations for waveform frequency utilities,
  • expected ISCO frequency behavior with spin,
  • peak-finding correctness,
  • polarization reconstruction and symmetry properties,
  • amplitude, phase, and frequency extraction,
  • mismatch normalization and orthogonality limits,
  • smoothness and endpoint behavior of blended waveforms,
  • correctness of phase alignment operations,
  • stability and positivity of legacy fitting functions.

Overall, this PR improves regression protection and numerical validation across the waveform generation and hybridization codebase while keeping the tests deterministic and lightweight for CI.

@prayush prayush assigned prayush and unassigned prayush May 8, 2026
@prayush prayush added the enhancement New feature or request label May 8, 2026
@prayush
prayush force-pushed the add_unit_testing branch from 9c32aa3 to 7aac119 Compare May 8, 2026 09:13
@prayush

prayush commented May 8, 2026

Copy link
Copy Markdown
Contributor Author

Note that all changes to files in esigmapy/ and esigmapy/surrogate/ are simply reformatting done with black. New code is only in tests/.

@prayush
prayush force-pushed the add_unit_testing branch from 246416a to aa4bc95 Compare May 14, 2026 12:24

@Akash-Maurya-0899 Akash-Maurya-0899 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prayush, I have added some comments. The major one is regarding the black formatting test, which may simply fail due to different black versions used by the tests vs by a developer on their local machine. The fix would be to pin a particular version black for the tests, and also add it as an optional "developer" dependency of esigmapy. More details in the comments.

Comment thread tests/test_formatting.py
Comment thread tests/test_blend.py Outdated
# 1.5 lies between arr[1]=1 and arr[2]=2; nearest is equidistant → either valid
arr = np.array([0.0, 1.0, 2.0, 3.0])
idx = find_first_value_location_in_series(arr, 1.5)
assert idx in (1, 2)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it check if the output is 1, and not 1 or 2 (because that's what find_first_value_location_in_series would output I think)? Or is it that due to floating point rounding error, it can be either 1 or 2?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, find_first_value_location_in_series is finding the first location in the first input (array) which has its value closest to the requested value. So in this case both are valid. But your point is right, i.e. in the very special case that the desired value is right in the middle of two known values, what do we do? I have updated both the find_first_value_location_in_series function and its test here to now really return the "first" value location in such cases.

Comment thread .github/workflows/tests.yml Outdated

- name: Install dependencies
run: |
pip install lalsuite pycbc pytest black

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pinned

Comment thread tests/test_blend.py
Comment thread tests/test_blend.py
Comment thread tests/test_blend.py
Comment thread tests/test_blend.py Outdated
# |3 + 4j| == 5
wave = np.full(10, 3.0 + 4.0j)
amp = compute_amplitude(wave)
assert np.allclose(amp, 5.0)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps decrease the tolerances rtol and atol in np.allclose everywhere to make the tests more stringent.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment thread tests/test_blend.py
Comment thread tests/test_mr_generator.py
Comment thread tests/test_mr_generator.py
@prayush
prayush force-pushed the add_unit_testing branch 3 times, most recently from 153ef57 to 9db62e2 Compare June 12, 2026 09:35
@prayush

prayush commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@Akash-Maurya-0899 all your comments have been addressed. If satisfied, approve this but please don't merge it yet.

@prayush
prayush force-pushed the add_unit_testing branch from 9db62e2 to 9603f74 Compare June 12, 2026 09:45
prayush added 3 commits June 12, 2026 15:17
  and mocked coverage for inspiral and merger-ringdown generators

- Add unit tests for inspiral and MR mode generation utilities using
mocked lalsimulation / delegated generators.
- Add deterministic unit tests for waveform utilities, blending helpers,
and legacy fitting functions without using mocks.

utils.py
* verify f22/x roundtrip consistency across x and mass ranges
* verify f22 scales as M^-1
* verify Schwarzschild ISCO frequency for zero spins
* verify prograde spins increase ISCO frequency
* verify retrograde spins decrease ISCO frequency
* verify monotonic inputs produce no detected peaks
* verify sinusoidal inputs return expected peak locations
* verify equatorial-plane polarization reconstruction for (2,2) mode
* verify face-on symmetry gives hx = 0
* verify polarization linearity under mode scaling

blend.py
* verify first-value lookup on constant arrays
* verify first/last index lookup on increasing arrays
* verify first/last symmetry properties
* verify amplitude extraction for known complex inputs
* verify amplitude for real-valued inputs
* verify phase unwrapping for linear complex phase
* verify constant-phase inputs produce flat phase
* verify frequency recovery from linear phase evolution
* verify decreasing phase produces negative frequencies
* verify mismatch is zero for identical waveforms
* verify mismatch is one for orthogonal waveforms
* verify mismatch remains within [0, 1]
* verify blended series matches endpoints correctly
* verify blended series smoothness across transition region
* verify blended series output length
* verify phase alignment reduces mismatch
* verify inverse alignments cancel net phase shift

legacy.py
* verify zero polynomial coefficients return unity
* verify quadratic polynomial evaluation for known coefficients
* verify equal numerator/denominator ratio polynomial returns unity
* verify fitting functions remain positive for η ∈ (0, 0.25]
* verify fitting functions are finite at η = 0.01 and η = 0.25

Included test coverage:

* TestGetInspiralEsigmaModesWithMock (7 tests)
  - verify returned dict contains requested (l,m) key
  - verify include_conjugate_modes=True/False controls (l,-m)
  - verify default return type is pycbc.TimeSeries
  - verify return_pycbc_timeseries=False returns numpy arrays
  - verify distance conversion from Mpc to SI before C call
  - verify mode generation called once per unique mode

* TestEccentricityAtReferenceFrequencyWithMock (2 tests)
  - verify nearest-index lookup logic for x_ref
  - verify returned eccentricity matches argmin|x - x_ref|

* TestGenerateLalsimModesWithMock (5 tests)
  - verify missing approximant propagates AttributeError -> IOError
  - verify linked-list traversal filters requested modes only
  - verify all requested modes are returned
  - verify unrequested modes are ignored
  - verify extracted mode data is returned as numpy arrays

* TestGetMrModesWithMock (4 tests)
  - verify f_ref=None defaults to f_lower
  - verify coa_phase=None defaults to 0.0
  - verify NRSur7dq4 routes to _generate_lalsim_modes
  - verify SEOBNRv5HM routes to _generate_pyseobnr_modes

- Enable unit tests to be run on github

- Add unit test to ensure formatting with black
@prayush
prayush force-pushed the add_unit_testing branch from 9603f74 to 5f25824 Compare June 12, 2026 09:47
@prayush

prayush commented Jun 12, 2026

Copy link
Copy Markdown
Contributor Author

@Akash-Maurya-0899 this is now ready for "Rebase and Merge", if you are happy with the resolution of your comments. I have incorporated all changes you asked for.

@Akash-Maurya-0899
Akash-Maurya-0899 merged commit 81fef3f into gwnrtools:master Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants